fix(script-tag): Include React and ReactDOM as dependencies, add installViewer export #721

swederik · 2019-07-27T10:51:03Z

Addresses #2 and #3 from this Issue: #521

Open to changing the name, not sure if installViewer is what users would expect.

Script tag usage is a bit simpler now:

<div id="root"></div>

<script>
    const config = {
      // default: '/'
      routerBasename: '/',
      // default: ''
      relativeWebWorkerScriptsPath: '',
      servers: {
        dicomWeb: [
          {
            name: 'DCM4CHEE',
            wadoUriRoot: 'https://server.dcmjs.org/dcm4chee-arc/aets/DCM4CHEE/wado',
            qidoRoot: 'https://server.dcmjs.org/dcm4chee-arc/aets/DCM4CHEE/rs',
            wadoRoot: 'https://server.dcmjs.org/dcm4chee-arc/aets/DCM4CHEE/rs',
            qidoSupportsIncludeField: true,
            imageRendering: 'wadors',
            thumbnailRendering: 'wadors',
          },
        ],
      },
      i18n: {
        LOCIZE_PROJECTID: 'a8da3f9a-e467-4dd6-af33-474d582a0294',
        LOCIZE_API_KEY: null, // Developers can use this to do in-context editing. DO NOT COMMIT THIS KEY!
        USE_LOCIZE: true
      }
    }

    // TODO: This is still necessary
    window.config = config;
</script>
<script src="index.umd.js" crossorigin></script>

<script>
    // Note: Uses the DOM element with the ID "root" by default
    // One could also run installViewer(config, containerId)
    window.OHIFStandaloneViewer.installViewer(config)
</script>

…nstallViewer function to simplify script tag usage.

codecov · 2019-07-27T10:53:30Z

Codecov Report

Merging #721 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #721      +/-   ##
=========================================
- Coverage    0.68%   0.68%   -0.01%     
=========================================
  Files          84      85       +1     
  Lines        1595    1602       +7     
  Branches      224     226       +2     
=========================================
  Hits           11      11              
- Misses       1362    1367       +5     
- Partials      222     224       +2

Impacted Files	Coverage Δ
src/index_publish.js	`0% <0%> (ø)`
src/config.js	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 210c2e8...18f5afa. Read the comment docs.

dannyrb · 2019-07-28T13:55:41Z

rollup.config.js

+          'PureComponent',
+          'useRef',
+          'useReducer',
+          'useMemo',


It's weird that this is a thing

Yeah it's a pain, I don't know why this is necessary.

dannyrb · 2019-07-28T13:57:06Z

src/config.js

+if (process && process.env && process.env.PUBLIC_URL) {
+  homepage = process.env.PUBLIC_URL;
+}
+
 window.info = {


If we're attaching anything to the window with script-tag, you may want to vendor prefix:
$ohif.info

If only to reduce chances of conflict

This doesn't change anything we are attaching, it just changes how process is checked, since it is undefined sometimes.

We need to get away from using window.config entirely and use Context or Redux to pass that data around. This can be another PR though.

dannyrb · 2019-07-28T13:57:58Z

src/index_publish.js

+	}
+
+	return ReactDOM.render(<App {...props}/>, container, callback);
+}


I think users will appreciate this. Thank you 🙌

dannyrb

Nothing jumped out at me. These seem like solid value adds.
Great work, Erik! ^_^

ohif-bot · 2019-07-29T08:56:34Z

🎉 This PR is included in version 0.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix(script-tag): Include React and ReactDOM as dependencies and add i…

18f5afa

…nstallViewer function to simplify script tag usage.

swederik changed the title ~~fix(script-tag): Include React and ReactDOM as dependencies and add i…~~ fix(script-tag): Include React and ReactDOM as dependencies, add installViewer export Jul 27, 2019

swederik requested a review from dannyrb July 27, 2019 10:52

dannyrb reviewed Jul 28, 2019

View reviewed changes

dannyrb approved these changes Jul 28, 2019

View reviewed changes

swederik merged commit 5802cf1 into master Jul 29, 2019

swederik deleted the fix/script-tag-improvements branch July 29, 2019 06:20

ohif-bot added the released label Jul 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(script-tag): Include React and ReactDOM as dependencies, add installViewer export #721

fix(script-tag): Include React and ReactDOM as dependencies, add installViewer export #721

swederik commented Jul 27, 2019 •

edited

Loading

codecov bot commented Jul 27, 2019 •

edited

Loading

dannyrb Jul 28, 2019

swederik Jul 29, 2019

dannyrb Jul 28, 2019

swederik Jul 29, 2019

dannyrb Jul 28, 2019

dannyrb left a comment

ohif-bot commented Jul 29, 2019

fix(script-tag): Include React and ReactDOM as dependencies, add installViewer export #721

fix(script-tag): Include React and ReactDOM as dependencies, add installViewer export #721

Conversation

swederik commented Jul 27, 2019 • edited Loading

codecov bot commented Jul 27, 2019 • edited Loading

Codecov Report

dannyrb Jul 28, 2019

Choose a reason for hiding this comment

swederik Jul 29, 2019

Choose a reason for hiding this comment

dannyrb Jul 28, 2019

Choose a reason for hiding this comment

swederik Jul 29, 2019

Choose a reason for hiding this comment

dannyrb Jul 28, 2019

Choose a reason for hiding this comment

dannyrb left a comment

Choose a reason for hiding this comment

ohif-bot commented Jul 29, 2019

swederik commented Jul 27, 2019 •

edited

Loading

codecov bot commented Jul 27, 2019 •

edited

Loading